Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Runtime snapshot runtime tests of Layout Animation on fabric #6538

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Latropos
Copy link
Contributor

@Latropos Latropos commented Sep 23, 2024

Summary

Snapshots of layout animations were not implemented for fabric (as layout animations were not implemented). In this PR I enable JS snapshots for fabric layout animations.

Test plan

@Latropos Latropos changed the title Runtime tests on fabric Runtime tests of Layout Animation on fabric Oct 14, 2024
@Latropos Latropos changed the title Runtime tests of Layout Animation on fabric Runtime snapshot runtime tests of Layout Animation on fabric Oct 14, 2024
@Latropos Latropos marked this pull request as ready for review October 14, 2024 09:55
// TODO Implement native snapshots for layout animations on Fabric
_updateNativeSnapshot([{ tag, update }], jsUpdates.value.length - 1);
} else {
brokenNativeSnapshotsOnFabric.value = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can safely move this assignment to the createUpdatesContainer method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would have some negative side effects, for example it would broke this test:

Cause only in case of layout animations the native snapshots are unimplemented, so we can assume that if function notifyAboutProgress has never been called, then native snapshots work fine on fabric.

Comment on lines +148 to +150
if (_IS_FABRIC && (-1) in sortedUpdates) {
return sortedUpdates[-1];
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something we need to verify. Does it happen all the time or just sometimes? Additionally, it will break in cases where we want to test animations for more than one component.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it happens all the time and breaks some of our animations. However most of our tests features only one component.

Comment on lines +22 to +25
if ((key as string) === 'opacity' && jsValue === 1 && nativeValue === undefined) {
// undefined opacity may get translated into 1, as it is the default value
valuesAreMatching = true;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, here we need to verify why in the JS update, we can receive undefined values

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In our layout animations we use function maybeRestoreOpacity a lot. After the restoration the opacity is explicitly defined as 1, even if it had been undefined.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants